[Store] Allow id to be int|string|Uuid for VectorDocument and TextDocument#867
[Store] Allow id to be int|string|Uuid for VectorDocument and TextDocument#867MolloKhan wants to merge 152 commits intosymfony:mainfrom
id to be int|string|Uuid for VectorDocument and TextDocument#867Conversation
MolloKhan
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
added
Feature
|
that should also be changed in |
|
I think this is it |
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
One issue we have now is for users not using the Uid component but receiving Uuid instances even if they used string before.
I think the best solution here is to just drop the dependency to the component. no feature of the component itself - besides the type-declaration - actually needs a Uuid and it was a wrong assumption of mine in the beginning based on starting with Pinecone as store 🙈
|
Good thinking. I'd need only to remove the Uuid auto-casting from the constructor, right? |
|
I removed the Uuid auto-casting and only left the type declaration |
OskarStark
changed the title
id to be int|string|Uuid for VectorDocument and TextDocument
|
Can we simplify something in the |
wouldn't know what, don't think so. but i think a valid follow up would be to kill UUID dependency in store component |
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
Can you please rebase this once and have another look at MongoDb, Weaviate, Typesense, Milvus, MariaDb, and Meilisearch?
I think your changes in bridge implementations for removing the Uuid::fromString(...) make sense, but is not done everywhere.
Thanks already @MolloKhan!
|
friendly ping @MolloKhan |
|
Sorry for the delay! I just got back home from holidays (it was such a long ride 😄) - I searched for all places where we cast strings into |
MolloKhan
force-pushed
the
vectordocument-support-types
branch
from
0b8cd10 to
59de316
Compare
MolloKhan
force-pushed
the
vectordocument-support-types
branch
from
7e8e863 to
7f9ed01
Compare
|
I don't know why the PR history shows a long list of commits, but I think this one is ready |
|
We can check if everything works by require 0.3 for store bridges in demo/composer.json file and populate the store like described in the demo/README.md |
|
I installed the demo and made it require |
|
Just require it an push, the CI will use your new code. On local you can use the link binary in the root of the project. |
|
Ohh cool! I wasn't aware of the link binary |
|
Alright, this is working locally, and tests are passing |
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
Thanks a lot @MolloKhan! 🙏
|
Reviewed & tested, but cannot merge with this history - running into conflicts. I guess due to unrelated commits on the branch. Could you please isolate your patch again? |
chr-hertel
left a comment
chr-hertel
left a comment
There was a problem hiding this comment.
Also needs a note in the CHANGELOG for store component 0.3 please
|
I surely screwed up something when rebasing. I'll create a clean PR |
|
Closing in favor #1397 |
…ocument` and `TextDocument` (MolloKhan) This PR was squashed before being merged into the main branch. Discussion ---------- [Store] Allow `id` to be `int|string|Uuid` for `VectorDocument` and `TextDocument` | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | no | Issues | Fix #857 | License | MIT This PR is a duplicate of #867 but with a clean commit history Commits ------- 263b7ed [Store] Allow `id` to be `int|string|Uuid` for `VectorDocument` and `TextDocument`
20 participants
I gave this a second thought, and I think @OskarStark idea is the way to go (#857 (comment))
If the ID coming from the store is in Uuid format the user will likely cast it themself
If you are ok with my changes, I'll proceed to update all the places where
VectorDocumentsare instantiated